-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Media based port settings in SONIC #328
Conversation
|
||
"PORT_OPTICS_SETTINGS": { | ||
|
||
"Ethernet20": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Ethernet20": As discussed on the call, I think it would be good to allow port lists (e.g. "Ethernet1, Ethernet3, Ethernet5 etc") and port ranges (e.g. "Ethernet1-Ethernet48") here. A list can also include range entries. I think this may make the file much shorter and more usable/maintainable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ben-gale . Agreed. Will add this to the document
|
||
"preemphasis": [ | ||
|
||
"0x1201", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain what these values represent please? They look like register contents, in which case, are these values similar/common in semantics and type across transceivers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the best abstractions of relevant pre-emp, idriver and pre-driver registers that are used by vendors. These will be common in semantics across transceivers. ASIC vendors have APIs to program these. So, it's upto the libsaiASIC-VENDOR implementation to implement these to map to their SDKs
|
||
"0x1" | ||
|
||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you think about adding a FEC setting here (for copper transceivers)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FEC can already be provided in port_config.ini (e.g. wnc/x86_64-wnc_osw1800-r0/OSW1800-48x6q/port_config.ini). FEC settings should be coming from config DB through the configuration present in port_config.ini
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct me if I'm wrong but is not the FEC set in the INTERFACE section, at least that is how we do it for specific optics
"Ethernet36": {
...
"fec": "rs"
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct me if I'm wrong but is not the FEC set in the INTERFACE section, at least that is how we do it for specific optics
"Ethernet36": {
...
"fec": "rs"
},
Hi. What i meant to convey here is there are multiple other mechanisms used to configure FEC and hence this specific mechanism need not have provision to configure FEC settings
|
||
"PORT_OPTICS_SETTINGS": { | ||
|
||
"Ethernet20": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As also discussed on the call, the file must also support Port Breakout scenarios - this may change the port references in the file. Please describe how will this be handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The document will be updated on breakout scenario.
Rename Media-based-Port-settings.md to doc/Media-based-Port-settings.md Update Media-based-Port-settings.md Update Media-based-Port-settings.md Update Media-based-Port-settings.md Update Media-based-Port-settings.md Update Media-based-Port-settings.md Update Media-based-Port-settings.md Image files Rename doc/Media-based-Port-settings.md to doc/media-settings/Media-based-Port-settings.md Delete event_flow.png Delete key_selection_flow.png Add files via upload Update Media-based-Port-settings.md Update Media-based-Port-settings.md Addressing the global range option Adding a global range block where the common values for multiple ports can be specified as a range. Changing indentation Update Media-based-Port-settings.md Update Media-based-Port-settings.md Update Media-based-Port-settings.md Update Media-based-Port-settings.md
33cddec
to
30a1500
Compare
as part of the development, please also add a checker program to make sure the media setting file submit by vendor is correct. here is an example. https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-device-data/tests/config_checker |
Sure Guohan. Will add that during development |
Updating the document based on latest design
Breakout Scenario: | ||
================== | ||
|
||
For breakout the particular media, if it has a global value is listed in the media_settings.json with appropriate lane values. If the values are specific to a port, then the entry is listed under the port. If the logical port is going to be created after breakout (E.g Ethernet0 is modified to Ethernet0,Ethernet1,Ethernet2,Ethernet3) those corresponding ports are listed in the ports section in media_settings.json. In the current breakout scenario, when port_config.ini is modified and a config reload is done, the flow will be similar to bootup sequence and thus all the media settings will be pushed from xcvrd to portsorch on restart during config apply. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you give detailed example for breakout scenario. have difficult to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lguohan I have given an example of breakout. I have modified the design of media settings file slightly so that the users do not need to do anything additionally to handle breakout scenario
can you format the doc? it is not well formatted. |
Media settings file: | ||
==================== | ||
|
||
Each vendor need to define the media settings file under device/<vendor-name>/ <ONIE_PLATFORM_STRING>/ <HARDWARE_SKU>/media_settings.json. This file contains the list of optics and DAC media-based settings for each port in the device. Each media setting for a media type comprises of key-value pairs per lane that is expected to be programmed when an optics or DAC is used. Examples of key value pairs include pre-emphasis, idriver and ipredriver. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you are going to handle break out scenario, the media_settings should not be related to HARDWARE_SKU, it should only related to the true hardware platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lguohan I have modified the document to have the file under platform path rather than SKU path. I have also slightly changed design so as to handle breakout cases
Formatting the document
Editing file for proper indentation
Explaining the breakout scenario with examples
Hi @lguohan I had done some formatting now. Please let me know if this looks good |
The Pre-emphasis settings can be different, when the port operates in different speeds. How does the definition handle it? |
Hi,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For pre-emphasis, these abstractions be further be classified into specific TX_FIR setting values(like pre-tap, main, post_tap). Some vendor-types choose to have either 3-tap co-effs or more based on the different modulation type. The order of values in plain hex string would be misleading at times.
(This was supposed to be a review comment. Realised, it took as approval by mistake. Plz Ignore) |
Hi. Currently SAI doesn't have TX_FIR individual attributes. I think the Vendor SAI can handle decoding pre-emphasis values into TX_FIR setting values. |
Actually there could cases that we can run different speed on same transceiver. e.g people can run 40G on a 100G optic. Seems we do not have such supported yet, but could we consider add in json schema? |
Hi, |
The tuning value will change if speed changed even with same transceiver module. |
More comments:
|
I assume most optics/DAC do not support dual speeds. Even if few does, I assume this might not be a normal use case where the media is used at a lower than possible speed. Please let me know your views. |
dgsudharsan, that's right, we do not see many cases that same optics/DAC support dual speed. We do not necessary have it support in this change. Just want make sure we have this fact considered and we can extend our schema if this case happens later. |
Hi @stevenlu99
|
Modified few places according to latest design.
Updated with latest design. Included default at global level, supporting multiple ranges at global level.
@dgsudharsan - Sorry to bother you. I want to add some functionality on your PR for media serdes attributes and wanted to make sure my idea is complete and don't break yours. I want to add: SAI_PORT_ATTR_HW_PROFILE_ID to SONiC to support other vendors and not have to add new SAI attributes for the various ASIC types. Since SONiC has quite a few repos just wanted to piggyback on yours and wanted to make sure I have all of them: #328 Did these also already handle the port-breakout scenario (multiple logical ports per physical port)? |
This pull request contains the high level document explaining the design of media based port settings in Sonic